-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ELEMENTS-1216: add route resolver to nuxeo-routing-behavior #271
Conversation
View issue in JIRA: ELEMENTS-1216: Allow nuxeo-document-suggestion to use both uid and path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a breaking change, we need to make it clear in the release notes, no?
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
10f0469
to
f9ddae7
Compare
f9ddae7
to
1669297
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
1669297
to
3af37e2
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
3af37e2
to
11d1b7c
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
The improvements seem clear. |
We are no longer introducing a breaking change here. We are now checking first if there is a route resolver defined, on nuxeo-document-suggestion, and if not, we keep the old behavior. |
11d1b7c
to
f530a87
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
} | ||
let entityType = obj['entity-type']; | ||
if (!entityType) { | ||
// XXX Sometimes we don't have the entity type. For example, on nuxeo-document-storage, we were not storing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we fix this at nuxeo-document-storage
level so we store the entity-type and add it in case it's missing on stored documents so we can do without this hack here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could certainly fix this for newly added documents, but I'm not quite sure how to fix this for already existing documents in a reliable way, since we just forward the value
of the inner iron-localstorage
. We do have a get
method which we use on some elements, but I don't think it would cover all use cases, specially those that rely on data binding.
Alternatively, we could try to do something with a new private property to hold the "fixe" set of documents, or we could try to run something when the value changes to fix the documents, but I'm not sure it is worth the effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to fix this on nuxeo-home, which is the only place by default where we generate URLs from docs stored on local storage, AFAIK. But I'm not sure I'd call this robust.
}; | ||
|
||
function setNuxeoRouterKey(entityType, value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remark: we really need a window.Nuxeo.UI.config.set('...')
or something...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
f530a87
to
d745a81
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
// document` method and do not know how to handle paths | ||
fn = (routeKey === 'path' && this.router.browse) || fn; | ||
} | ||
routeKey = routeKey || 'id'; // let `id` be the default key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎵 let id
be 🎵 let id
be ...
a4d70a3
d745a81
to
a4d70a3
Compare
⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here |
Required by nuxeo/nuxeo-web-ui#985.
This introduces a breaking change to people developing on top of the nuxeo-elements alone. People using on top of Web UI will not be affected. It can be fixed by either locking into a previous version or manually fixing their router.